Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow the Emulator user to specify a User ID #1456

Merged
merged 12 commits into from Apr 26, 2019
Merged

Allow the Emulator user to specify a User ID #1456

merged 12 commits into from Apr 26, 2019

Conversation

denscollo
Copy link
Contributor

@denscollo denscollo commented Apr 24, 2019 •
edited by Aliandi

Fixes #1344

Description

This PR allows the Emulator users to specify a User ID which will be used within conversations.

Changes made

Functionality changes

To implement this feature we made the following changes:

  • Add an input to the Emulator Settings so that the user can configure an ID
  • Change the startNewConversation method to choose between a randomly generated UserId, the UserId set in the props or the UserId set by the user.
  • When the Use custom id checkbox is set to false, the input is disabled and cleaned. When true, it is a required field and it gets enabled.
  • The User ID field has a validation so that the user is not able to enter anything other than a GUID.
  • Restarting a conversation with the same userId will choose between the one set by the user or the one set by default (if the first one is empty).
  • Restarting a conversation with a new userId will generate a random ID the same way it was doing it until now.
  • Remove Restart Conversation Split Button
  • Add two new buttons Restart with Same User Id and Restart with New User Id to replace Restart Conversation Split Button.
  • Add validation to the Save button to avoid setting the require user ID flag as true but not setting an ID

UI Changes

Before
imagen
imagen

After
imagen
imagen

Testing

Manual testing
BETranscript-Test

Automated testing
Added automated tests for the new settings options.

imagen

@coveralls
Copy link

coveralls commented Apr 24, 2019 •
edited

Coverage Status

Coverage increased (+0.02%) to 57.053% when pulling ece6fb4 on feature/southworks/userid into 1185b06 on master.

justinwilaby
justinwilaby suggested changes Apr 24, 2019
View reviewed changes
Copy link
Contributor

@justinwilaby justinwilaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small changes to consider. Overall it looks good

packages/app/client/src/ui/editor/appSettingsEditor/appSettingsEditor.tsx Outdated Show resolved Hide resolved
packages/app/client/src/ui/editor/appSettingsEditor/appSettingsEditor.tsx Outdated Show resolved Hide resolved
packages/app/client/src/ui/editor/emulator/emulator.tsx Outdated Show resolved Hide resolved
tonyanziano
tonyanziano suggested changes Apr 24, 2019
View reviewed changes
Copy link
Contributor

@tonyanziano tonyanziano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some styling issues:

The "User ID" input field label needs to have disabled styling (lighter color) when the input field is disabled.

image

Right now, the color makes the input appear to be active when it is actually disabled. The label, "User ID," should use the same color treatment as the checkbox label under "Data Collection" when it is disabled:

image

This feedback will make it easier for users to understand that the checkbox is actually enabling / disabling the text field.

Also, I've spoken to @mewa1024 about the "Restart Conversation" button in the livechat toolbar, and we have decided to keep it as one button with an alternative option via dropdown, the way it was previously. So it should be changed back to a single <SplitButton> instead of two separate buttons.

image

@Aliandi
Copy link
Contributor

Aliandi commented Apr 25, 2019

Ok @justinwilaby and @tonyanziano ! We are working on the feedback!

@tonyanziano
Copy link
Contributor

tonyanziano commented Apr 25, 2019 •
edited

The text field label looks good when disabled, but the disabled state persists when the text field is enabled. The color of the label should change depending on the state of the text field.

image

Right now:

disabled -> light gray
enabled -> same light gray

Desired behavior:

disabled -> light gray
enabled -> darker gray

@denscollo
Copy link
Contributor Author

Hi @tonyanziano We are still working on the Label Color feedback that you gave us and the one about the getDerivedStateFromProps function. As for the User Id Label, we are having some issues with the disabled property of the label but we are working on it.

@tonyanziano
Copy link
Contributor

Sounds good! @denscollo 👍

justinwilaby
justinwilaby suggested changes Apr 26, 2019
View reviewed changes
Copy link
Contributor

@justinwilaby justinwilaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some very small housekeeping issues.

packages/app/client/src/ui/editor/appSettingsEditor/appSettingsEditor.tsx Outdated Show resolved Hide resolved
packages/app/client/src/ui/editor/appSettingsEditor/appSettingsEditor.tsx Outdated Show resolved Hide resolved
justinwilaby
justinwilaby previously approved these changes Apr 26, 2019
View reviewed changes
@tonyanziano
Copy link
Contributor

Nice job, thanks!

@justinwilaby justinwilaby merged commit 4422c69 into master Apr 26, 2019
@justinwilaby justinwilaby deleted the feature/southworks/userid branch April 26, 2019 21:24
@tonyanziano tonyanziano mentioned this pull request May 31, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aliandi Aliandi Aliandi left review comments

@tonyanziano tonyanziano tonyanziano approved these changes

@justinwilaby justinwilaby justinwilaby left review comments

@a-b-r-o-w-n a-b-r-o-w-n Awaiting requested review from a-b-r-o-w-n

@compulim compulim Awaiting requested review from compulim

@corinagum corinagum Awaiting requested review from corinagum

@cwhitten cwhitten Awaiting requested review from cwhitten

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow the Emulator user to specify a User ID to be used for conversations
5 participants
@denscollo @coveralls @Aliandi @tonyanziano @justinwilaby